Update for containerization #32
Conversation
WalkthroughA new Go file introduces a Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for "github.com/conductorone/baton-sdk/pkg/field"" Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mbburch
force-pushed
the
mbburch/bb-733-lambda-prep
branch
from
25aafac to
8f239d1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/config/conf.gen.go (2)
13-26: Consider adding error context to findFieldByTag.The reflection code correctly implements dynamic field lookup, but when fields aren't found, there's no context about what was being searched for.
-func (c* Postgresql) findFieldByTag(tagValue string) (any, bool) { +func (c* Postgresql) findFieldByTag(tagValue string) (any, bool) { v := reflect.ValueOf(c).Elem() // Dereference pointer to struct t := v.Type() for i := 0; i < t.NumField(); i++ { field := t.Field(i) tag := field.Tag.Get("mapstructure") if tag == tagValue { return v.Field(i).Interface(), true } } return nil, false }
1-87: Consider adding caching to improve performance.The current implementation uses reflection for every field lookup, which can be inefficient for frequently accessed fields. Consider implementing a cache for the reflection results.
type Postgresql struct { Dsn string `mapstructure:"dsn"` Schemas []string `mapstructure:"schemas"` IncludeColumns bool `mapstructure:"include-columns"` IncludeLargeObjects bool `mapstructure:"include-large-objects"` // Cache for field lookups fieldCache map[string]reflect.StructField } // Initialize the cache in a constructor func NewPostgresql() *Postgresql { p := &Postgresql{ fieldCache: make(map[string]reflect.StructField), } // Pre-populate the cache t := reflect.TypeOf(p).Elem() for i := 0; i < t.NumField(); i++ { field := t.Field(i) tag := field.Tag.Get("mapstructure") if tag != "" { p.fieldCache[tag] = field } } return p }This would replace the need for the
findFieldByTagmethod with a more efficient cached lookup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.github/workflows/release.yamlis excluded by none and included by none.goreleaser.docker.yamlis excluded by none and included by noneDockerfile.lambdais excluded by none and included by noneMakefileis excluded by none and included by nonecmd/baton-postgresql/main.gois excluded by none and included by nonepkg/config/gen/gen.gois excluded by!**/gen/**and included bypkg/**
📒 Files selected for processing (2)
pkg/config/conf.gen.go(1 hunks)pkg/config/config.go(2 hunks)
🔇 Additional comments (4)
pkg/config/config.go (2)
1-1: Package name change is appropriate for modularization.The change from
maintoconfigpackage name aligns well with the containerization effort, allowing this configuration code to be properly imported as a module.
16-17: Good use of code generation for configuration.The addition of the
go:generatedirective and renaming/exporting of the configuration variable provides a clean interface for accessing configuration values. This approach supports the containerization efforts by making the configuration more modular and accessible.pkg/config/conf.gen.go (2)
1-12: Good structure for PostgreSQL configuration.The
Postgresqlstruct with mapstructure tags correctly maps to the configuration fields defined inconfig.go. This provides a type-safe way to access configuration values.
1-2: Generated code warning is appropriate.The warning comment clearly indicates this is generated code that shouldn't be manually edited, which is good practice.
| func (c *Postgresql) GetStringSlice(fieldName string) []string { | ||
| v, ok := c.findFieldByTag(fieldName) | ||
| if !ok { | ||
| return []string{} | ||
| } | ||
| t, ok := v.([]string) | ||
| if !ok { | ||
| panic("wrong type") | ||
| } | ||
| return t | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning errors instead of panicking.
The getter methods currently panic when a type assertion fails. In production code, it's generally better to return errors that can be handled gracefully rather than causing the application to crash.
-func (c *Postgresql) GetString(fieldName string) string {
+func (c *Postgresql) GetString(fieldName string) (string, error) {
v, ok := c.findFieldByTag(fieldName)
if !ok {
- return ""
+ return "", fmt.Errorf("field with tag %q not found", fieldName)
}
t, ok := v.(string)
if !ok {
- panic("wrong type")
+ return "", fmt.Errorf("field %q is not of type string", fieldName)
}
- return t
+ return t, nil
}You would need to update the other getter methods similarly and import the fmt package.
Also applies to: 40-50, 52-62, 64-74, 76-86
🤖 Prompt for AI Agents
In pkg/config/conf.gen.go around lines 28 to 38, the GetStringSlice method
panics when the type assertion fails, which can crash the application. Refactor
this method to return an error instead of panicking by changing its signature to
return ([]string, error), returning the slice and nil on success, and returning
nil with a descriptive error using fmt.Errorf when the type assertion fails or
the field is not found. Apply similar changes to the other getter methods in the
specified line ranges and import the fmt package to support error formatting.
| // Code generated by baton-sdk. DO NOT EDIT!!! | ||
| package config | ||
|
|
||
| import "reflect" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing imports for proper error handling.
If you implement the error handling suggestion above, you'll need to add the fmt package to your imports.
-import "reflect"
+import (
+ "fmt"
+ "reflect"
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import "reflect" | |
| import ( | |
| "fmt" | |
| "reflect" | |
| ) |
🤖 Prompt for AI Agents
In pkg/config/conf.gen.go at line 4, the import statement currently only
includes "reflect". To support proper error handling as suggested, add the "fmt"
package to the import list. This will enable formatting error messages correctly
in your code.
Summary by CodeRabbit
New Features
Chores